-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BorderControl: Render border color/style dropdown as UnitControl prefix #42212
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice cleanup PR @aaronrobertshaw! This is testing well for me in Storybook and the different widths (compact and custom width), and the component still looks and functions the same in the editor via a Group block and the border controls.
LGTM! ✨
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm very happy to see things getting simplified 🎉
/* Prevent unit select forcing min height larger than its UnitControl */ | ||
min-height: 0; | ||
${ rtl( | ||
{ | ||
borderRadius: '0 1px 1px 0', | ||
marginRight: 0, | ||
}, | ||
{ | ||
borderRadius: '1px 0 0 1px', | ||
marginLeft: 0, | ||
} | ||
{ borderRadius: '0 2px 2px 0' }, | ||
{ borderRadius: '2px 0 0 2px' } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand what these styles are for 🤔 Are they no longer needed...?
The :focus
stuff too, should they be moved to UnitControl
itself? (Not necessarily in this PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I swear that when I was putting this PR together I still needed these styles or the unit select showed square corners. I must have been seeing things. After retesting the border-radius rules can be removed.
While testing the focus styles it looks like the focus styling in general for the UnitControl
is a little broken e.g. wrapper's border overlays the unit selects focus border/box-shadow etc.
I've moved these focus styles to the UnitControl
in a separate PR in the hope that might provide a better historical record if we need to revisit them in the future. See #42383.
For this PR, I'll update it shortly to target the #42383 branch as its base and simply remove the overrides from the BorderControl
.
41dce38
to
ffc8d0d
Compare
Nice PR, too bad it was ignored by the 6.0.1 update from earlier today. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
2cfb9f2
to
4a6a3e6
Compare
ffc8d0d
to
9974f64
Compare
9974f64
to
498d1df
Compare
What?
Renders the
BorderControl
dropdown as aprefix
within itsUnitControl
.Why?
Rendering as a prefix on the UnitControl allows us to reduce some of the CSS overrides.
How?
UnitControlProps
type to allow aReactNode
for theprefix
prop.UnitControl
'sprefix
.HStack
wrapper and cleans up/fixes stylesTesting Instructions
npm run build:package-types
BorderControl
unit tests still pass:npm run test-unit packages/components/src/border-control/test
BorderBoxControl
unit tests pass:npm run test-unit packages/components/src/border-box-control/test
BorderControl
&BorderBoxControl
isCompact
andwidth
props.